fix(security): address VS Code extension vulnerabilities — prototype pollution, token exposure, ReDoS, missing CSP#489
Merged
imran-siddique merged 5 commits intomicrosoft:mainfrom Mar 28, 2026
Conversation
- mcp-proxy: shebang must be line 1 (TS18026) - copilot, mcp-server: typescript ^6.0.2 → ^5.7.0 (eslint <6.0.0) - NuGet: replace ESRP Sign+Release with NuGetCommand@2 push via NuGet.org service connection Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#486, microsoft#487, microsoft#488) Prototype pollution (HIGH): - WorkflowDesignerPanel._updateNode() now uses property allowlist instead of Object.assign — blocks __proto__/constructor injection OAuth token exposure (HIGH): - ssoProvider stores tokens in VS Code SecretStorage (encrypted) instead of globalState (plaintext JSON on disk) - _loadState() restores tokens from secrets, _saveState() strips them ReDoS via custom policy regex (MEDIUM): - Added safeRegExp() that rejects patterns with known catastrophic backtracking constructs before compilation - Invalid patterns are skipped with warning, not compiled Missing CSP on 5 webview panels (MEDIUM): - Added Content-Security-Policy meta tags with nonce-based script-src to WorkflowDesigner, PolicyEditor, MetricsDashboard, Onboarding, and CMVK panel in extension.ts Dead code removal (LOW): - Deleted policyCompletion.ts (361 lines, unused, superseded by language/completionProvider.ts) Closes microsoft#486, Closes microsoft#487, Closes microsoft#488 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🤖 AI Agent: security-scanner — Security Review of Pull Request: microsoft/agent-governance-toolkitSecurity Review of Pull Request: microsoft/agent-governance-toolkit1. Prompt Injection Defense Bypass
2. Policy Engine Circumvention
3. Trust Chain Weaknesses
4. Credential Exposure
5. Sandbox Escape
6. Deserialization Attacks
7. Race Conditions
8. Supply Chain Weaknesses
SummaryThe pull request addresses several critical security issues, particularly around credential management and prototype pollution. However, there are still significant concerns regarding prompt injection, policy circumvention, and supply chain vulnerabilities that need to be addressed to ensure the security integrity of the agent governance toolkit. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request addresses several critical security vulnerabilities in the VS Code extension of the microsoft/agent-governance-toolkit repository. The fixes include addressing prototype pollution, securing OAuth tokens, mitigating ReDoS attacks, adding a Content Security Policy (CSP) to webviews, and removing dead code. While the fixes are a step in the right direction, there are some areas that require further scrutiny and improvement.
🔴 CRITICAL Issues
-
Prototype Pollution Mitigation
- Issue: The fix for prototype pollution involves adding a property allowlist in
_updateNode. However, the implementation details are not shown in the diff. If the allowlist is not comprehensive or if there are other entry points for untrusted input, the vulnerability may persist. - Action: Ensure that the allowlist is exhaustive and that all potential entry points for untrusted input are addressed. Add unit tests to verify that the allowlist prevents prototype pollution.
- Issue: The fix for prototype pollution involves adding a property allowlist in
-
OAuth Token Handling
- Issue: While migrating to the VS Code
SecretStorageAPI is a good step, there is no evidence in the diff that the migration has been fully implemented and tested. Additionally, there is no mention of how existing plaintext tokens are being securely migrated to the new storage mechanism. - Action: Verify that the migration to
SecretStorageis complete and that existing tokens are securely migrated. Add tests to ensure tokens are securely stored and retrieved.
- Issue: While migrating to the VS Code
-
ReDoS Mitigation
- Issue: The
safeRegExp()validator is mentioned as a fix for ReDoS vulnerabilities, but the implementation is not visible in the diff. Without seeing the implementation, it is unclear whether the validator is robust enough to handle all edge cases. - Action: Review the
safeRegExp()implementation to ensure it effectively mitigates ReDoS attacks. Add tests with known ReDoS patterns to validate its effectiveness.
- Issue: The
-
Content Security Policy (CSP)
- Issue: Adding nonce-based CSP meta tags is a good step, but there is no evidence in the diff that the CSP is being dynamically generated with unique nonces for each webview instance. A static nonce would not provide adequate protection.
- Action: Verify that the CSP implementation generates unique nonces for each webview instance. Add tests to ensure that the CSP is applied correctly and that it blocks inline scripts without the correct nonce.
🟡 Warnings
-
Potential Breaking Changes
- Issue: Migrating OAuth tokens to the
SecretStorageAPI could potentially break existing functionality if not handled carefully, especially if users have existing tokens stored in plaintext. - Action: Document the migration process clearly for users. Provide a fallback mechanism or a migration script to ensure a smooth transition.
- Issue: Migrating OAuth tokens to the
-
Dependency Updates
- Issue: The
package-lock.jsonfile shows updates to multiple dependencies, including major version bumps (e.g.,@typescript-eslint/eslint-pluginto6.21.0). These updates could introduce breaking changes. - Action: Review the changelogs of updated dependencies to ensure compatibility with the current codebase. Run comprehensive tests to verify that the updates do not introduce regressions.
- Issue: The
💡 Suggestions
-
Dead Code Removal
- Suggestion: While removing
policyCompletion.tsis a good step, ensure that this file is not being used indirectly or in any legacy workflows. Add a note in the PR description about how this was verified.
- Suggestion: While removing
-
Documentation
- Suggestion: Update the documentation to reflect the changes made in this PR, especially for the migration to
SecretStorageand the addition of CSP.
- Suggestion: Update the documentation to reflect the changes made in this PR, especially for the migration to
-
Testing
- Suggestion: Add unit and integration tests for all the fixes implemented in this PR. Specifically:
- Tests for the
safeRegExp()validator with both safe and malicious regex patterns. - Tests for the CSP implementation to ensure it blocks inline scripts and only allows scripts with the correct nonce.
- Tests for the
SecretStorageAPI to ensure tokens are securely stored and retrieved.
- Tests for the
- Suggestion: Add unit and integration tests for all the fixes implemented in this PR. Specifically:
-
Code Review
- Suggestion: Provide a more detailed description of the changes made in the PR, especially for critical fixes like prototype pollution and ReDoS mitigation. This will help reviewers understand the scope and effectiveness of the fixes.
-
Security Audit
- Suggestion: Consider conducting a security audit of the entire VS Code extension to identify any other potential vulnerabilities.
Conclusion
The PR addresses critical security vulnerabilities, but the lack of visibility into the implementation details and the absence of tests make it difficult to fully assess the effectiveness of the fixes. Addressing the critical issues and warnings outlined above will help ensure the robustness and security of the changes.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request addresses critical security vulnerabilities in the VS Code extension of the microsoft/agent-governance-toolkit repository. The fixes include resolving prototype pollution, securing OAuth tokens, mitigating ReDoS vulnerabilities, implementing a Content Security Policy (CSP), and removing dead code. While the fixes address significant security concerns, there are some areas that require further review and improvement.
🔴 CRITICAL
-
Prototype Pollution Fix
- The fix for prototype pollution involves adding a property allowlist in
_updateNode. However, the PR does not include the actual implementation of this fix in the diff provided. Ensure that:- The allowlist is comprehensive and explicitly defines all allowed properties.
- The implementation prevents prototype pollution by rejecting any unexpected or malicious properties.
- Add unit tests to verify that the allowlist works as intended and that no prototype pollution is possible.
- The fix for prototype pollution involves adding a property allowlist in
-
OAuth Token Storage
- The migration to the VS Code
SecretStorageAPI is a good step forward. However:- Ensure that the token is encrypted at rest and is only accessible to the extension.
- Add tests to verify that the token is securely stored and cannot be accessed by unauthorized code.
- Confirm that the token is not logged or exposed in any debug logs or error messages.
- The migration to the VS Code
-
ReDoS Mitigation
- The
safeRegExp()validator is introduced to mitigate ReDoS vulnerabilities. However:- The implementation of
safeRegExp()is not visible in the diff. Ensure that it properly detects and rejects all potentially catastrophic regular expressions. - Add tests to verify that the validator blocks dangerous patterns (e.g., exponential backtracking) while allowing safe ones.
- The implementation of
- The
-
Content Security Policy (CSP)
- Adding nonce-based CSP meta tags to the webview panels is a good security measure. However:
- Verify that the CSP is strict enough to prevent XSS attacks. For example:
- Ensure that
script-srconly allows scripts with the specified nonce and does not include unsafe directives likeunsafe-inlineorunsafe-eval. - Ensure that
object-srcis set to'none'unless explicitly required.
- Ensure that
- Add tests to verify that the CSP is applied correctly to all webview panels.
- Verify that the CSP is strict enough to prevent XSS attacks. For example:
- Adding nonce-based CSP meta tags to the webview panels is a good security measure. However:
🟡 WARNING
- Dead Code Removal
- The removal of
policyCompletion.ts(361 lines) may cause backward compatibility issues if any other part of the codebase or external integrations depend on it. Verify that:- No other parts of the codebase reference the removed file.
- There are no external dependencies or users relying on this file.
- If this is a breaking change, document it clearly in the release notes.
- The removal of
💡 SUGGESTIONS
-
Testing
- Ensure that comprehensive tests are added for all the fixes, especially for the
safeRegExp()validator and the CSP implementation. - Consider adding integration tests to verify the end-to-end behavior of the extension after these changes.
- Ensure that comprehensive tests are added for all the fixes, especially for the
-
Documentation
- Update the documentation to reflect the changes, especially for the migration to the
SecretStorageAPI. Include instructions for developers on how to use the new API.
- Update the documentation to reflect the changes, especially for the migration to the
-
Dependency Management
- The
package-lock.jsonfile has been added, but it is unclear if all dependencies are necessary. Review the dependencies to ensure that there are no unused or vulnerable packages. - Consider using a tool like
npm auditto identify and address any vulnerabilities in the dependencies.
- The
-
Code Review
- The diff provided is truncated, making it difficult to review the full implementation of the fixes. Ensure that the complete code changes are reviewed for correctness and security.
-
Versioning
- The version has been updated to
1.0.1. Ensure that this version bump aligns with your versioning policy and reflects the changes made in this PR.
- The version has been updated to
Summary of Actions Required
- Provide the implementation details for the prototype pollution fix and
safeRegExp()validator. - Add unit and integration tests for all fixes.
- Review and document the removal of
policyCompletion.tsfor potential breaking changes. - Verify the security of the CSP implementation and the
SecretStorageAPI usage. - Review the dependency tree for unnecessary or vulnerable packages.
- Ensure the version bump aligns with your versioning policy.
Let me know if you need further assistance with any of these points!
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Pull Request Review: Security Fixes for VS Code Extension
Summary
This pull request addresses five security vulnerabilities in the VS Code extension of the microsoft/agent-governance-toolkit repository. The fixes include addressing prototype pollution, token exposure, Regular Expression Denial of Service (ReDoS), missing Content Security Policy (CSP), and removal of dead code.
🔴 CRITICAL
-
Prototype Pollution via
Object.assign- Issue: The fix involves adding a property allowlist to
_updateNode. However, the implementation of the allowlist is not visible in the provided diff. - Risk: If the allowlist is not properly implemented or if it misses critical properties, the vulnerability could still be exploited.
- Action: Ensure that the allowlist implementation is comprehensive and explicitly prevents prototype pollution. Add unit tests to verify that no unintended properties can be added to the object.
- Issue: The fix involves adding a property allowlist to
-
OAuth Token in Plaintext
globalState- Issue: The migration to the VS Code
SecretStorageAPI is a good step. However, the diff does not provide details on how the migration is handled. - Risk: If the migration process does not securely transfer existing tokens from
globalStatetoSecretStorage, there is a risk of token leakage. - Action: Verify that tokens stored in
globalStateare securely migrated toSecretStorageand thatglobalStateis cleared afterward. Add tests to confirm this behavior.
- Issue: The migration to the VS Code
-
ReDoS via Custom Policy Regex
- Issue: The introduction of a
safeRegExp()validator is a good approach. However, the implementation ofsafeRegExp()is not visible in the diff. - Risk: If the validator does not comprehensively detect and reject dangerous patterns, the ReDoS vulnerability may persist.
- Action: Review the implementation of
safeRegExp()to ensure it effectively mitigates ReDoS risks. Add tests with known ReDoS patterns to validate the fix.
- Issue: The introduction of a
-
Missing CSP on Webview Panels
- Issue: Adding nonce-based CSP meta tags is a good practice. However, the diff does not show how the nonces are generated and applied.
- Risk: If nonces are not securely generated or are reused, the CSP could be bypassed.
- Action: Verify that nonces are generated securely (e.g., using a cryptographically secure random number generator) and are unique for each request. Add tests to ensure CSP headers are correctly applied to all webview panels.
🟡 WARNING
- Dead Code Removal
- Issue: The removal of
policyCompletion.tsdeletes 361 lines of code. While this is marked as "dead code," it is not clear if this file is used in any indirect way or if its removal could impact backward compatibility. - Risk: If this file is referenced in any other part of the codebase or by external consumers, its removal could cause runtime errors or break existing integrations.
- Action: Confirm that
policyCompletion.tsis not referenced anywhere in the codebase or by external dependencies. If it is part of the public API, consider deprecating it first before removal.
- Issue: The removal of
💡 SUGGESTIONS
-
Testing Coverage
- Add unit and integration tests to validate the fixes for all five issues. Specifically:
- Test the
safeRegExp()function with a variety of safe and unsafe regex patterns. - Test the migration of tokens from
globalStatetoSecretStorageto ensure no tokens are left in plaintext. - Test the CSP implementation to ensure it blocks inline scripts and only allows scripts with the correct nonce.
- Test the
- Add unit and integration tests to validate the fixes for all five issues. Specifically:
-
Documentation
- Update the documentation to reflect the changes made in this PR, especially regarding the new use of the
SecretStorageAPI and the CSP implementation.
- Update the documentation to reflect the changes made in this PR, especially regarding the new use of the
-
Dependency Audit
- The
package-lock.jsonfile has been added in this PR, which includes a large number of dependencies. Ensure that all dependencies are up-to-date and free of known vulnerabilities. Use tools likenpm auditorsnykto verify.
- The
-
Code Review for Non-Visible Changes
- The provided diff does not show the actual code changes for the fixes. Ensure that the implementation of the fixes (e.g.,
safeRegExp(), CSP nonces, and token migration) is reviewed for correctness and security.
- The provided diff does not show the actual code changes for the fixes. Ensure that the implementation of the fixes (e.g.,
-
Backward Compatibility
- If the removed
policyCompletion.tsfile is part of the public API, consider adding a deprecation warning before removing it completely. This will give users time to update their code.
- If the removed
Final Recommendation
- Address the CRITICAL issues before merging this PR.
- Verify the implementation of the fixes and add comprehensive tests.
- Ensure backward compatibility for the removed
policyCompletion.tsfile. - Perform a dependency audit to ensure no new vulnerabilities are introduced.
Let me know if you need further assistance or clarification!
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes 5 security issues reported in #486, #487, #488.
TypeScript compiles cleanly.
Closes #486, Closes #487, Closes #488